Workspace CI job clean up by joostjager · Pull Request #4459 · lightningdevkit/rust-lightning

joostjager · 2026-03-04T12:40:29Z

No description provided.

ldk-reviews-bot · 2026-03-04T12:40:36Z

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull · 2026-03-04T14:03:48Z

ci/ci-tests-features.sh

@@ -6,12 +6,10 @@ source "$(dirname "$0")/ci-tests-common.sh"

 echo -e "\n\nChecking and testing lightning with features"


If you remove the check part, you might want to update the printed message here and elsewhere to remove the Checking part.

Removed. I left it in place in ci-tests-sync.sh, because there is still some only-checking in non CI.

tnull · 2026-03-04T14:09:05Z

ci/ci-tests-sync.sh


 if [ -z "$CI_ENV" ] && [[ -z "$BITCOIND_EXE" || -z "$ELECTRS_EXE" ]]; then
 	echo -e "\n\nSkipping testing Transaction Sync Clients due to BITCOIND_EXE or ELECTRS_EXE being unset."
-	cargo check -p lightning-transaction-sync --tests


Why was this line dropped?

codecov · 2026-03-05T09:47:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.09%. Comparing base (90b79e4) to head (c0cdaaa).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4459      +/-   ##
==========================================
+ Coverage   85.97%   86.09%   +0.12%     
==========================================
  Files         159      159              
  Lines      104722   105185     +463     
  Branches   104722   105185     +463     
==========================================
+ Hits        90030    90559     +529     
+ Misses      12191    12115      -76     
- Partials     2501     2511      +10

Flag	Coverage Δ
tests	`86.09% <ø> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkczyz

Review courtesy of Claude

jkczyz · 2026-03-05T17:27:07Z

ci/ci-tests-sync.sh

 # shellcheck source=ci/ci-tests-common.sh
 source "$(dirname "$0")/ci-tests-common.sh"

 echo -e "\n\nChecking and testing Block Sync Clients with features"


Drop "Checking and ".

I left it in place (#4459 (comment)), but also fine with removing. Removed.

jkczyz · 2026-03-05T17:31:08Z

ci/ci-tests-workspace.sh

+cargo check --tests --quiet --color always

-WORKSPACE_MEMBERS=( $(cat Cargo.toml | tr '\n' '\r' | sed 's/\r    //g' | tr '\r' '\n' | grep '^members =' | sed 's/members.*=.*\[//' | tr -d '"' | tr ',' ' ') )
-
-echo -e "\n\nTesting the workspace, except lightning-transaction-sync."
+echo -e "\n\nTesting the workspace."
 cargo test --quiet --color always


Are these two redundant?

I think the goal is to fail as fast as possible - perhaps 🤷‍♂️

jkczyz · 2026-03-05T17:32:03Z

ci/ci-tests-sync.sh

+	cargo check -p lightning-transaction-sync --tests --quiet --color always --features esplora-async-https
+	cargo check -p lightning-transaction-sync --tests --quiet --color always --features electrum
 else
 	echo -e "\n\nTesting Transaction Sync Clients with features."


Should this also test without any features?

We should indeed be consistent. The crate doesn't contains anything without a feature it seems (lib.rs), but maybe we do want to check that it doesn't error. Added the test without features. diff

ldk-reviews-bot · 2026-03-07T08:40:38Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-03-09T08:41:36Z

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

cargo check does something different than cargo test. It doesn't check with test-specific features enabled and thus can catch bugs where we have code that only builds with test features. I don't think these should be removed (they're also incredibly cheap).

Replace the per-member cargo check + cargo doc loop with a single `cargo doc --workspace` call. The per-member cargo check is redundant with the workspace-level cargo check already run earlier in the script. Also remove the separate `cargo test -p lightning-custom-message` which is covered by the workspace-level cargo test. Fix stale "except lightning-transaction-sync" echo messages, as it has been a workspace member for a while now. AI tools were used in preparing this commit.

joostjager · 2026-03-10T11:14:21Z

cargo check does something different than cargo test. It doesn't check with test-specific features enabled and thus can catch bugs where we have code that only builds with test features. I don't think these should be removed (they're also incredibly cheap).

I think you mean cargo check checks #[cfg(not(test))]? Good point. The goal wasn't really speed up here, but just simplification. Reverted.

That leaves just the workspace build simplification.

joostjager marked this pull request as ready for review March 4, 2026 13:30

ldk-reviews-bot requested a review from jkczyz March 4, 2026 13:30

tnull reviewed Mar 4, 2026

View reviewed changes

joostjager force-pushed the ci-cleanup-redundant-checks branch from 03629f0 to 6b0eeea Compare March 5, 2026 08:08

joostjager requested a review from tnull March 5, 2026 08:40

joostjager added this to Weekly Goals Mar 5, 2026

joostjager self-assigned this Mar 5, 2026

jkczyz reviewed Mar 5, 2026

View reviewed changes

joostjager force-pushed the ci-cleanup-redundant-checks branch 2 times, most recently from ab5a336 to c0cdaaa Compare March 6, 2026 09:32

joostjager requested a review from jkczyz March 6, 2026 10:24

jkczyz previously approved these changes Mar 6, 2026

View reviewed changes

TheBlueMatt reviewed Mar 9, 2026

View reviewed changes

joostjager dismissed jkczyz’s stale review via b35a849 March 10, 2026 11:14

joostjager force-pushed the ci-cleanup-redundant-checks branch from c0cdaaa to b35a849 Compare March 10, 2026 11:14

joostjager changed the title ~~Remove redundant cargo check/test calls from CI scripts~~ Workspace CI job clean up Mar 10, 2026

		@@ -6,12 +6,10 @@ source "$(dirname "$0")/ci-tests-common.sh"

		echo -e "\n\nChecking and testing lightning with features"

Conversation

joostjager commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Mar 7, 2026

Uh oh!

ldk-reviews-bot commented Mar 9, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joostjager commented Mar 4, 2026 •

edited

Loading

ldk-reviews-bot commented Mar 4, 2026 •

edited

Loading

codecov bot commented Mar 5, 2026 •

edited

Loading